rts/Game/Camera.h (1)

205-205: Fix indentation inconsistency and remove extra blank line.

The new additions use 4-space indentation while the rest of the file appears to use tabs. Additionally, line 315 is an unnecessary blank line.

🔎 Proposed fix

-    float GetTerrainDistance() const { return terrainDistance; }
+	float GetTerrainDistance() const { return terrainDistance; }

-    void TraceToTerrain();
+	void TraceToTerrain();

-    float terrainDistance;
-
+	float terrainDistance;

Also applies to: 257-257, 314-315

rts/Game/Camera.cpp (1)

714-731: Consider refactoring for consistency and clarity.

A few optional improvements:

• The magic number 30000 could be a named constant (e.g., MAX_TERRAIN_TRACE_DISTANCE)
• Indentation uses 4 spaces instead of tabs, inconsistent with the rest of the file

🔎 Suggested improvements

Define a constant at the top of the file:

constexpr float MAX_TERRAIN_TRACE_DISTANCE = 30000.0f;

Then use it in the call:

     terrainDistance = TraceRay::GuiTraceRay(
         GetPos(),
         GetForward(),
-        30000,
+        MAX_TERRAIN_TRACE_DISTANCE,
         nullptr,
         hitUnit,
         hitFeature,
         true,
         true,
         true
     );

Also fix indentation to use tabs consistently with the rest of the file.

rts/System/Sound/CMakeLists.txt (1)

11-11: Minor: Inconsistent indentation style.

The added lines use spaces for indentation, while the surrounding code uses tabs. Consider aligning with the existing style for consistency.

-        AttenuationModels/RtsAttenuationModel.cpp
+		AttenuationModels/RtsAttenuationModel.cpp

Also applies to: 46-46

rts/System/Sound/OpenAL/SoundSource.cpp (2)

142-144: Unused variable: gain is retrieved but never used.

The alGetSourcef call reads the gain value into gain, but the variable is never used afterward. Either remove this dead code or use the value for validation/logging.

-    float gain;
-
-    alGetSourcef(id, AL_GAIN, &gain);

---

79-88: Consider placing helper functions in an anonymous namespace.

SmoothTowards and Curve are free functions at file scope. Using an anonymous namespace limits their visibility to this translation unit and avoids potential ODR issues.

+namespace {
+
 float SmoothTowards(float current, float target, float speed, float dt)
 {
     const float t = 1.0f - std::exp(-speed * dt);
     return current + (target - current) * t;
 }

 float Curve(float t, float min, float max, float k)
 {
     return min + (max - min) * std::pow(t, k);
 }
+
+} // namespace

rts/System/Sound/OpenAL/SoundSource.h (1)

152-152: Potential performance concern: config lookup on every call.

UseAttenuationModel() reads from configHandler each time it's called, including during Update() which runs frequently. Consider caching this value and updating it only when the config changes.

+    bool useAttenuationModelCached = false;
+
-    bool UseAttenuationModel() { return configHandler->GetBool("snd_useAttenuationModel"); }
+    bool UseAttenuationModel() const { return useAttenuationModelCached; }

Then update the cached value in CSound::ConfigNotify or during initialization.

rts/System/Sound/OpenAL/Sound.h (2)

13-13: Prefer including the interface header rather than the concrete implementation.

Including RtsAttenuationModel.h creates tight coupling. Since GetAttenuationModel() returns ISoundAttenuationModel*, the header only needs the interface. The concrete type can be forward-declared or included only in the .cpp file.

-#include "System/Sound/AttenuationModels/RtsAttenuationModel.h"
+#include "System/Sound/ISoundAttenuationModel.h"

---

29-30: Minor: Redundant public: section.

There are two consecutive public: sections. Consider merging DrawDebug() into the existing public section below.

rts/System/Sound/AttenuationModels/RtsAttenuationModel.h (2)

14-18: Performance concern: Config lookups on every Evaluate() call.

These getter methods read from configHandler each time Evaluate() is called. Since Evaluate() runs for every sound source during updates, this could become a performance bottleneck. Consider caching these values and updating them via ConfigNotify.

+private:
+    // Cached config values - updated via ConfigNotify
+    float forwardAttenuationRange = 5000.0f;
+    float backwardAttenuationRange = 1000.0f;
+    float outerAttenuationRange = 2000.0f;
+    float minVolumeAttenuation = 0.0f;
+    float minFilterAttenuation = 0.0f;
+
+public:
+    void UpdateFromConfig();
+
 private:
-    float GetForwardAttenuationRange() const { return configHandler->GetFloat("snd_forwardAttenuationRange"); }
-    float GetBackwardAttenuationRange() const { return configHandler->GetFloat("snd_backwardAttenuationRange"); }
-    float GetOuterAttenuationRange() const { return configHandler->GetFloat("snd_outerAttenuationRange"); }
-    float GetMinVolumeAttenuation() const { return configHandler->GetFloat("snd_minVolumeAttenuation"); }
-    float GetMinFilterAttenuation() const { return configHandler->GetFloat("snd_minFilterAttenuation"); }
+    float GetForwardAttenuationRange() const { return forwardAttenuationRange; }
+    float GetBackwardAttenuationRange() const { return backwardAttenuationRange; }
+    float GetOuterAttenuationRange() const { return outerAttenuationRange; }
+    float GetMinVolumeAttenuation() const { return minVolumeAttenuation; }
+    float GetMinFilterAttenuation() const { return minFilterAttenuation; }

---

21-21: Move descriptive comment to class documentation.

The trailing comment describing the RTS model would be more useful as a Doxygen-style comment above the class declaration.

+/**
+ * RTS Attenuation Model
+ *
+ * Designed for RTS games with viewport-based attenuation, forward/backward
+ * distance handling, off-screen attenuation, and zoom-aware center attenuation.
+ * Similar to sound systems in games like "Planetary Annihilation" or "Supreme Commander 2".
+ */
 class RtsAttenuationModel : public ISoundAttenuationModel
 {
 ...
 };
-
-// "RTS Model: Designed for RTS games with viewport-based attenuation..."

rts/System/Sound/AttenuationModels/RtsAttenuationModel.cpp (3)

7-7: Unused include: TraceRay.h.

The TraceRay.h header is included but no symbols from it appear to be used in this file.

-#include "Game/TraceRay.h"

---

63-64: Simplify pow(forwardValue, 1).

std::pow(forwardValue, 1) is equivalent to forwardValue. Simplify for clarity.

-        out.volumeFactor = std::pow(forwardValue, 6) * outerValue;
-        out.filterFactor = std::pow(forwardValue, 1) * outerValue;
+        out.volumeFactor = std::pow(forwardValue, 6) * outerValue;
+        out.filterFactor = forwardValue * outerValue;

---

56-58: Complex expression is difficult to read and verify.

This nested ternary with clamp and lerp is hard to follow. Consider breaking it into named intermediate values for clarity.

-        float forwardValue = std::clamp(out.forwardDistance >= 0 ?
-            std::lerp(GetMinVolumeAttenuation(), 1.0f, 1.0f - out.forwardDistance / GetForwardAttenuationRange()) :
-            std::clamp(1.0f - (-out.forwardDistance) / GetBackwardAttenuationRange(), 0.0f, 1.0f), GetMinVolumeAttenuation(), 1.0f);
+        float forwardValue;
+        if (out.forwardDistance >= 0) {
+            // Sound is in front of camera
+            float t = 1.0f - out.forwardDistance / GetForwardAttenuationRange();
+            forwardValue = std::lerp(GetMinVolumeAttenuation(), 1.0f, t);
+        } else {
+            // Sound is behind camera
+            float t = 1.0f - (-out.forwardDistance) / GetBackwardAttenuationRange();
+            forwardValue = std::clamp(t, 0.0f, 1.0f);
+        }
+        forwardValue = std::clamp(forwardValue, GetMinVolumeAttenuation(), 1.0f);

rts/System/Sound/ISoundAttenuationModel.h (3)

6-6: Address or remove the TODO-style comment.

This comment suggests incomplete design. Either implement the additional fields now or create a tracked issue and remove the comment to avoid leaving unactionable TODOs in the codebase.

Do you want me to open an issue to track this potential enhancement?

---

9-9: Update the comment to match the actual struct fields.

The comment mentions "totalFactor" but the struct contains volumeFactor and filterFactor instead. Update the comment to accurately describe what the struct contains.

---

22-29: Consider adding documentation for the public API.

The interface and its Evaluate method lack documentation comments. Adding brief doc comments would help future developers understand the purpose of the attenuation model and the contract of the Evaluate method.

📝 Example documentation

+/// Interface for sound attenuation models that calculate spatial audio parameters
+/// based on listener position and orientation.
 class ISoundAttenuationModel {
 public:
     virtual ~ISoundAttenuationModel() = default;
 
+    /// Evaluates attenuation parameters for a sound source.
+    /// @param in Input containing the sound source position
+    /// @return Output containing attenuation factors and spatial data
     virtual SoundAttenuationOutput Evaluate(
         const SoundAttenuationInput& in
     ) const = 0;
 };

rts/System/Sound/ISound.cpp (1)

44-46: Add validation constraints and specify units.

These distance/range configs lack minimumValue() constraints, unlike other configs in this file (see lines 30-38). Consider adding .minimumValue(0.0f) or appropriate bounds to prevent invalid negative distances.

Additionally, specify the units in the descriptions (e.g., "Distance in game units" or "Distance in elmos").

🔎 Proposed constraints

-CONFIG(float, snd_forwardAttenuationRange).defaultValue(8000).description("Distance in front of the camera over which sound volume and clarity gradually fade.");
-CONFIG(float, snd_backwardAttenuationRange).defaultValue(300).description("Distance behind the camera over which sounds are attenuated.");
-CONFIG(float, snd_outerAttenuationRange).defaultValue(500).description("How far sounds can be outside the camera view before being strongly attenuated.");
+CONFIG(float, snd_forwardAttenuationRange).defaultValue(8000.0f).minimumValue(0.0f).description("Distance (in game units) in front of the camera over which sound volume and clarity gradually fade.");
+CONFIG(float, snd_backwardAttenuationRange).defaultValue(300.0f).minimumValue(0.0f).description("Distance (in game units) behind the camera over which sounds are attenuated.");
+CONFIG(float, snd_outerAttenuationRange).defaultValue(500.0f).minimumValue(0.0f).description("How far sounds can be (in game units) outside the camera view before being strongly attenuated.");

Configuration used: defaults

Review profile: CHILL

Plan: Pro

rts/System/Sound/OpenAL/SoundSource.h (1)

121-121: Remove unused member variables cameraZoom and lastUpdate.

Based on past review analysis, these member variables are declared but never used in the implementation. They should be removed to avoid confusion.

🔎 Suggested removal

-    float cameraZoom;
-
     float3 currentPosition = float3(0.0f, 0.0f, 0.0f);
...
 	spring_time loopStop {1e9};
-    spring_time lastUpdate = spring_gettime();
 	bool in3D = false;

Also applies to: 136-136

rts/System/Sound/OpenAL/Sound.cpp (1)

1118-1135: Thread safety concern: DrawDebug() accesses soundSources without locking.

This method iterates over soundSources which can be modified by the sound thread. Other methods like Update() and GetNextBestSource() acquire soundMutex before accessing this collection. Consider adding synchronization to prevent data races.

🔎 Suggested fix

 void CSound::DrawDebug() const
 {
+    std::lock_guard<spring::recursive_mutex> lck(soundMutex);
+
     // Only draw 3D sound sources that are currently playing
     for (const CSoundSource& source: soundSources) {

Note: This requires either making soundMutex mutable or removing the const qualifier from DrawDebug().

rts/Game/Camera.h (3)

205-205: LGTM! Accessor follows established pattern.

The accessor is consistent with other distance getters like GetNearPlaneDist() and GetFarPlaneDist(). Optionally, consider adding a brief documentation comment explaining the purpose (distance from camera to terrain intersection point).

---

257-257: Private method is appropriate for internal camera updates.

The method is correctly declared as private, as it's called internally during the camera update cycle. Optionally, consider adding a documentation comment describing that this method traces from the camera to terrain and updates terrainDistance.

---

314-315: Document the default terrain distance value.

The default value 1000 lacks a comment explaining its purpose. Consider adding inline documentation clarifying:

• This is the fallback distance before the first terrain trace completes
• The units are game units (consistent with attenuation range parameters like snd_forwardAttenuationRange = 8000)
• Why 1000 was chosen as the default (roughly 12.5% of the forward attenuation range)

This matters for the attenuation system in RtsAttenuationModel.cpp, which uses GetTerrainDistance() to compute zoom-based volume attenuation.

rts/System/Sound/OpenAL/SoundSource.h (2)

145-145: Consider moving UseAttenuationModel() to the .cpp file.

Including ConfigHandler.h in a header just for this inline helper adds compile-time dependencies to all files that include SoundSource.h. Since this is a private helper, moving it to the implementation file would be cleaner.

Additionally, calling configHandler->GetBool() on every frame for every source may have minor overhead. Consider caching the value or querying it once per frame at a higher level.

🔎 Suggested refactor

In SoundSource.h, declare the helper without the body:

-    bool UseAttenuationModel() { return configHandler->GetBool("snd_useAttenuationModel"); }
+    bool UseAttenuationModel() const;

In SoundSource.cpp, add the implementation:

bool CSoundSource::UseAttenuationModel() const {
    return configHandler->GetBool("snd_useAttenuationModel");
}

Then remove the #include "System/Config/ConfigHandler.h" from the header.

---

37-38: Inconsistent indentation style.

The new code uses 4-space indentation while the existing code uses tabs. This creates visual inconsistency. Consider aligning with the existing tab-based indentation style of the file.

Also applies to: 43-44, 60-60, 65-70

rts/System/Sound/OpenAL/Sound.cpp (1)

83-85: Consider using std::unique_ptr for automatic memory management.

While the current implementation correctly manages the attenuationModel lifecycle, using std::unique_ptr<ISoundAttenuationModel> would provide automatic RAII cleanup and eliminate the need for explicit delete calls.

rts/System/Sound/ISoundAttenuationModel.h (1)

10-19: Consider initializing SoundAttenuationOutput fields to prevent undefined behavior.

The struct fields are default-initialized (uninitialized for primitive types). While RtsAttenuationModel::Evaluate() currently sets all fields, future implementations or modifications could accidentally leave fields uninitialized, causing undefined behavior.

🔎 Suggested initialization

 struct SoundAttenuationOutput {
-    float forwardDistance;
-    float outerDistance;
-    float frustumHeight;
-    float frustumWidth;
-    float volumeFactor;
-    float filterFactor;
-    float tiltFactor;
-    float zoomFactor;
+    float forwardDistance = 0.0f;
+    float outerDistance = 0.0f;
+    float frustumHeight = 0.0f;
+    float frustumWidth = 0.0f;
+    float volumeFactor = 1.0f;  // Safe default: full volume
+    float filterFactor = 1.0f;  // Safe default: no filtering
+    float tiltFactor = 0.0f;
+    float zoomFactor = 0.0f;
 };

rts/System/Sound/OpenAL/SoundSource.cpp (3)

75-84: Consider making helper functions static or placing in anonymous namespace.

SmoothTowards and Curve are free functions that don't access any class state. They should be static or placed in an anonymous namespace to limit their visibility to this translation unit.

🔎 Suggested refactor

+namespace {
+
 float SmoothTowards(float current, float target, float speed, float dt)
 {
     const float t = 1.0f - std::exp(-speed * dt);
     return current + (target - current) * t;
 }

 float Curve(float t, float min, float max, float k)
 {
     return min + (max - min) * std::pow(t, k);
 }

+} // anonymous namespace

---

135-135: Add null check for currentChannel before dereferencing.

While currentChannel should be set when currentSoundItem.id != 0, adding a defensive null check would prevent potential crashes if the state becomes inconsistent.

🔎 Suggested fix

-    alSourcef(id, AL_GAIN, std::clamp(attenuationOutput.volumeFactor * currentChannel->GetVolume(), 0.0f, 1.0f));
+    const float channelVolume = (currentChannel != nullptr) ? currentChannel->GetVolume() : 1.0f;
+    alSourcef(id, AL_GAIN, std::clamp(attenuationOutput.volumeFactor * channelVolume, 0.0f, 1.0f));

---

81-84: Remove unused Curve function.

The Curve function is defined but never used in this file. Remove it to avoid dead code.

🔎 Suggested removal

-float Curve(float t, float min, float max, float k)
-{
-    return min + (max - min) * std::pow(t, k);
-}
-

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.

Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.

rts/System/Sound/OpenAL/SoundSource.cpp (2)

124-133: Address TODO and FIXME comments.

Multiple unresolved concerns are documented:

• Line 124-126: Random volume scaling needs redesign for normalized range
• Line 128: 2D events incorrectly affected by filters
• Line 130-131: Volume value conversion strategy unclear
• Line 133: Commented-out alternative gain calculation

These indicate uncertainty in the implementation approach.

Would you like me to open an issue to track these concerns, or help design a solution for the volume normalization and 2D/3D separation?

---

309-311: Remove unused gain retrieval.

The gain value is retrieved but never used. This appears to be leftover debug code.

🔎 Proposed cleanup

 	alSourcePlay(id);
-
-    float gain;
-    alGetSourcef(id, AL_GAIN, &gain);
-

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.

Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.

rts/System/Sound/OpenAL/SoundSource.cpp (4)

81-84: Dead code: Curve function is defined but never used.

This function is not called anywhere in the file. If it's intended for future use, consider adding a comment or removing it until needed.

---

137-139: Unused variable: gain is fetched but never used.

This appears to be leftover debug code. Either remove it or use it for logging/validation if intended.

-    float gain;
-
-    alGetSourcef(id, AL_GAIN, &gain);
-

---

309-310: Unused variable: gain is fetched but never used.

Same as in ApplyAttenuationModel - this appears to be leftover debug code. Consider removing it.

-    float gain;
-    alGetSourcef(id, AL_GAIN, &gain);
-

---

325-326: Consider resetting attenuationFilter to 0 after deletion.

For defensive coding, reset the handle after deletion to prevent potential double-delete issues or stale handle usage.

     if (attenuationFilter != 0)
+    {
         alDeleteFilters(1, &attenuationFilter);
+        attenuationFilter = 0;
+    }

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.

Learnt from: Majavaa
Repo: Majavaa/RecoilEngine PR: 3
File: rts/System/Sound/ISound.cpp:43-43
Timestamp: 2025-12-28T10:25:36.188Z
Learning: In rts/System/Sound/ISound.cpp, the snd_useAttenuationModel config intentionally defaults to false while the description recommends "ON". This is by design to recommend the feature without forcing users to opt in.